Skip to content

[dagster-airlift][jobs 6/n] jobs UI #29278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 1, 2025

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Apr 15, 2025

Summary & Motivation

I bit most of the implementation from #28997. The changes:

  • genericize the gql layer beyond airlift to "externalJobSource", allowing for multiple potential systems.
  • allow the overview timeline to make use of the icon/external stuff.

How I Tested These Changes

Live against kitchen sink.

Screen Recording 2025-04-23 at 4.01.28 PM.mov (uploaded via Graphite)

@dpeng817 dpeng817 requested review from OwenKephart and prha April 15, 2025 01:39
@dpeng817 dpeng817 marked this pull request as ready for review April 15, 2025 01:39
Copy link

github-actions bot commented Apr 15, 2025

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-acelharkq-elementl.vercel.app
https://airlift-jobs-ui.components-storybook.dagster-docs.io

Built with commit 5431920.
This pull request is being automatically deployed with vercel-action

Copy link

github-actions bot commented Apr 15, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-ehfwjldre-elementl.vercel.app
https://airlift-jobs-ui.core-storybook.dagster-docs.io

Built with commit 9e9b16f.
This pull request is being automatically deployed with vercel-action

@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 431a600 to cdf963d Compare April 15, 2025 16:35
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from cdf963d to 84e01c1 Compare April 15, 2025 17:04
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 84e01c1 to ab1be69 Compare April 15, 2025 18:14
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from ab1be69 to 9aad18e Compare April 18, 2025 17:32
@@ -114,7 +114,7 @@

POOL_TAG_PREFIX = f"{HIDDEN_TAG_PREFIX}pool/"

EXTERNAL_JOB_SOURCE_TAG_KEY = f"{SYSTEM_TAG_PREFIX}/external_job_source"
EXTERNAL_JOB_SOURCE_TAG_KEY = f"{SYSTEM_TAG_PREFIX}external_job_source"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be an inconsistency in how the external job source tag key is defined. The current definition:

EXTERNAL_JOB_SOURCE_TAG_KEY = f"{SYSTEM_TAG_PREFIX}external_job_source"

Since SYSTEM_TAG_PREFIX is defined as "dagster/" (with a trailing slash), this would create a tag key of "dagster/external_job_source".

However, in the frontend code's isExternalRun() function, it's looking for a tag key named "dagster/external_job_source".

This definition is correct, but it's worth noting that there was a duplicate definition later in the file (now removed) that had a different format with a hyphen:

EXTERNAL_JOB_SOURCE_TAG_KEY = f"{SYSTEM_TAG_PREFIX}/external-job-source"

This would have created a tag key of "dagster//external-job-source" with a double slash and hyphens instead of underscores.

The current implementation is correct, but it's important to ensure consistency in tag naming conventions throughout the codebase.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +78 to +83
useEffect(() => {
if (isExternal !== options.isExternal) {
setOptions({...options, isExternal});
}
return () => {};
}, [isExternal, options]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency array includes the entire options object, which can lead to unnecessary re-renders since objects are compared by reference. Consider using only the specific property needed for comparison:

useEffect(() => {
  if (isExternal !== options.isExternal) {
    setOptions({...options, isExternal});
  }
  return () => {};
}, [isExternal, options.isExternal]);

This ensures the effect only runs when the actual values being compared change.

Suggested change
useEffect(() => {
if (isExternal !== options.isExternal) {
setOptions({...options, isExternal});
}
return () => {};
}, [isExternal, options]);
useEffect(() => {
if (isExternal !== options.isExternal) {
setOptions({...options, isExternal});
}
return () => {};
}, [isExternal, options.isExternal])

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 6f8eda0 to e8ce65d Compare April 24, 2025 04:28
@dpeng817 dpeng817 force-pushed the airlift_jobs_ui branch 2 times, most recently from bcc288a to 2b05e81 Compare April 24, 2025 05:06
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from e8ce65d to cb63759 Compare April 24, 2025 23:47
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from cb63759 to 11e2003 Compare April 25, 2025 00:14
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 11e2003 to cb5f767 Compare April 25, 2025 00:29
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from cb5f767 to b127806 Compare April 25, 2025 02:02
This was referenced Apr 25, 2025
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from b127806 to 6119436 Compare April 29, 2025 00:46
@dpeng817 dpeng817 force-pushed the airlift_jobs_ui branch 2 times, most recently from b614ed6 to 32c91b8 Compare April 30, 2025 19:41
This was referenced Apr 30, 2025
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 6119436 to 6767a37 Compare April 30, 2025 20:46
@dpeng817 dpeng817 force-pushed the dpeng817/dpeng817/monitoring_job_real branch from 6767a37 to ed73075 Compare May 1, 2025 00:17
@dpeng817 dpeng817 force-pushed the airlift_jobs_ui branch from 84307cc to 8dfb8f3 Compare May 1, 2025 00:17
## Summary & Motivation
Pipes and airlift have kinda the same mandate here to use a
well-structured format for defining metadata in an external process,
that may not have access to the dagster package. So we can just move the
processing code to a shared place accessible to both, and make the
language more generic to instead be "external" metadata.

## How I Tested These Changes
Existing pipes metadata tests
@dpeng817 dpeng817 merged commit cf7faa0 into dpeng817/dpeng817/monitoring_job_real May 1, 2025
2 of 3 checks passed
@dpeng817 dpeng817 deleted the airlift_jobs_ui branch May 1, 2025 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants